Break up and function-ize handle_new_monitor_update
#4138
Conversation
We'll be breaking up this macro in upcoming commits.
|
👋 Thanks for assigning @joostjager as a reviewer! |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4138 +/- ##
==========================================
+ Coverage 88.62% 88.73% +0.11%
==========================================
Files 180 180
Lines 134895 135826 +931
Branches 134895 135826 +931
==========================================
+ Hits 119546 120528 +982
+ Misses 12587 12526 -61
- Partials 2762 2772 +10
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
valentinewallace
force-pushed
the
2025-10-monitor-upd-macro
branch
2 times, most recently
from
0e37cb0 to
57d0422
Compare
There was a problem hiding this comment.
Love to see the macro overloads disappear. Macros are already hard enough to deal with / navigate through without overloads.
I don't know how much you've been looking into (or already knew) what these macros do exactly, but any knowledge that you can solidify into additional comments seems very helpful to me.
| /// them (otherwise they can end up getting applied out-of-order) but it's not always possible to | ||
| /// drop the aforementioned peer state locks at a given callsite. In this situation, use this macro | ||
| /// to apply the monitor update immediately and handle the monitor update completion actions at a | ||
| /// later time. |
There was a problem hiding this comment.
Nice explanation. I strongly believe that adding comments like this will make the code base a lot easier to work with, especially for new people.
|
👋 The first review has been submitted! Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer. |
lightning/src/ln/channelmanager.rs
Outdated
| /// Returns whether the monitor update is completed, `false` if the update is in-progress. | ||
| fn handle_monitor_update_res<LG: Logger>( | ||
| &self, update_res: ChannelMonitorUpdateStatus, channel_id: ChannelId, logger: LG, | ||
| ) -> bool { |
There was a problem hiding this comment.
Not really convinced it makes sense to move some logs into a freestanding method? How does this improve readability? Or did you benchmark compilation?
There was a problem hiding this comment.
I am getting a bit faster compile times with this change, not sure what the bar is for "worthwhile improvement."
From a clean build:
valentine@Valentines-MacBook-Pro rust-lightning % hyperfine --prepare 'cargo clean' \
'cargo build -p lightning' \
'git stash && cargo build -p lightning'
Benchmark 1: cargo build -p lightning
Time (mean ± σ): 48.756 s ± 0.225 s [User: 135.911 s, System: 6.500 s]
Range (min … max): 48.448 s … 49.058 s 10 runs
Benchmark 2: git stash && cargo build -p lightning
Time (mean ± σ): 48.625 s ± 0.145 s [User: 135.571 s, System: 6.543 s]
Range (min … max): 48.436 s … 48.852 s 10 runs
Summary
git stash && cargo build -p lightning ran
1.00 ± 0.01 times faster than cargo build -p lightning
^Benchmark 1 reverts the new function back to a macro on this branch.
Incremental build time reverting to a macro:
valentine@Valentines-MacBook-Pro rust-lightning % hyperfine 'touch lightning/src/ln/chan_utils.rs && cargo build -p lightning'
Benchmark 1: touch lightning/src/ln/chan_utils.rs && cargo build -p lightning
Time (mean ± σ): 5.733 s ± 0.046 s [User: 5.007 s, System: 1.043 s]
Range (min … max): 5.671 s … 5.840 s 10 runs
Incremental build time with this current branch:
valentine@Valentines-MacBook-Pro rust-lightning % hyperfine 'touch lightning/src/ln/chan_utils.rs && cargo build -p lightning'
Benchmark 1: touch lightning/src/ln/chan_utils.rs && cargo build -p lightning
Time (mean ± σ): 5.625 s ± 0.020 s [User: 4.925 s, System: 1.038 s]
Range (min … max): 5.591 s … 5.649 s 10 runs
There was a problem hiding this comment.
Hmm, not a lot, but also not zero. Does it need to be a function in ChannelManager or can it be a loose function in channelmanager.rs to at least keep the code together in the same place?
There was a problem hiding this comment.
Switched to a loose function!
Generally we'd like to use functions instead of macros where we can both for readability and for compile times.
Makes handle_new_monitor_update more readable.
Makes handle_new_monitor_update more readable.
Makes handle_new_monitor_update more readable. Also documents the variant and why someone may want to use it.
Finishes work over the past few commits of breaking up the handle_new_monitor_update macro.
valentinewallace
force-pushed
the
2025-10-monitor-upd-macro
branch
from
57d0422 to
b588431
Compare
4 participants
Prefactor to some of @joostjager's experimental work including serialized channels in
ChannelMonitorUpdates as part of getting rid ofChannelManagerpersistence.